feat: append data with reserved stable row ids#6438
Conversation
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
majin1102
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@jackye1995 Hi, could you take a look at this issue and PR? If the motivation and proposal make sense to you, I’ll start a vote thread. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cd5bfaa7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match self.0 { | ||
| Operation::Append { fragments } => { | ||
| Operation::Append { fragments, .. } => { | ||
| let fragments = export_vec(py, fragments.as_slice())?; |
There was a problem hiding this comment.
After introducing Operation::ReserveRowIds, this IntoPyObject match still has no arm for it and falls through to the _ => todo!() branch later in the same function. That makes Dataset.read_transaction(...) / get_transactions(...) crash at runtime for any version created by a reserve-row-ids commit, which breaks Python transaction/history APIs as soon as such a dataset is encountered.
Useful? React with 👍 / 👎.
| match operation { | ||
| Operation::Append { | ||
| fragments: rust_fragments, | ||
| .. | ||
| } => { |
There was a problem hiding this comment.
This JNI conversion match does not include Operation::ReserveRowIds; it currently falls through to _ => unimplemented!() in convert_to_java_operation_inner. As a result, reading transactions from Java (for example nativeReadTransaction / transaction listing) will panic when a dataset version was produced by ReserveRowIds, causing hard failures instead of a usable Java operation object.
Useful? React with 👍 / 👎.
1 participant
Close #6466
What this PR does
This PR introduces a new
ReserveRowIdstransaction operation and extendsAppendwith an optionalrow_idsfield. Both changes affect transaction metadata and the serialized transaction schema, so this part needs a vote.What this PR does not do
This PR does not add any new field or persisted state to the manifest. The manifest format remains unchanged. The only manifest-related effect is that this PR provides a controlled transaction-level entry point for reserving and consuming
next_row_id, rather than changing manifest contents themselves.